-
-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix members being loaded from server on initial sync (defeating lazy loading) #3830
Conversation
@@ -1232,10 +1232,6 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv | |||
config, | |||
); | |||
} | |||
|
|||
// start tracking devices for any users already known to be in this room. | |||
const members = await room.getEncryptionTargetMembers(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what was triggering the loadMembersIfNeeded
. Was called during initial sync when the m.room.encryption was processed.
I moved that code in the RoomEncryptor, it's now only getting the known members from existing state and not loading if neeed
}); | ||
} | ||
|
||
// Query keys in case we don't have them for newly tracked members. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really related to the current problem, but I had to fix it to make the lazy loading test pass on both crypto backend. It's anyhow a real problem, and we can see several occurence of it in rageshakes. The (full) rust sdk as a similar mecanism to ensure the download of user keys for non tracked/dirty users.
This would need to be fixed in mobile too
element-hq/element-web#26439
spec/integ/crypto/crypto.spec.ts
Outdated
@@ -670,18 +670,21 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, | |||
}); | |||
|
|||
it("prepareToEncrypt", async () => { | |||
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} }); | |||
const homeserverUrl = "https://alice-server.com"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to modernize a bit the tests by using E2EKeyResponder, because we are requesting keys a bit differently and the existing test were using fragile tactis Alice will check Bob's device list (twice, because reasons)
.
I haven't done it all because it's a bit strange. In these test the olmTestDevice is sometime an alice device and sometimes a bob device.
If ok, I'd like to clean all this file in a following PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments on the other bits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're keen to merge this, go ahead. I'd prefer it to be rewritten without the recursion but won't insist.
Fixes element-hq/element-web#26418
Fixes element-hq/element-web#26439
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes